Skip to content

Fix crash in def_readwrite for non-smart-holder properties of smart-holder classes#6003

Open
virtuald wants to merge 1 commit intopybind:masterfrom
virtuald:enum-mismatch
Open

Fix crash in def_readwrite for non-smart-holder properties of smart-holder classes#6003
virtuald wants to merge 1 commit intopybind:masterfrom
virtuald:enum-mismatch

Conversation

@virtuald
Copy link
Contributor

@virtuald virtuald commented Mar 3, 2026 •
edited
Loading

Description

Ran into an issue where my extension was failing with pybind11 calling free on an enum (and on some platforms this ends up being a double-free?). Asked AI to dig into it and it got it wrong several times, but it finally did hit on a really subtle bug (described below).

Honestly, the fix and tests are probably slop -- I'm not sure why it decided to run the test in a subprocess, but the fix looks right and the tests pass. Open to suggestions.

I fixed the slop, looks better now.

gpt-5.3-high's analysis of the bug

This comes from a subtle interaction of generated bindings + smart_holder:

  1. AlertSim::AlertInfo is bound as py::class_<..., py::smart_holder>.
  2. AlertInfo.level is bound via def_readwrite("level", &AlertInfo::level).
  3. For smart-holder classes, pybind’s def_readwrite path for by-value members uses a std::shared_ptr aliasing getter.
  4. Here D is wpi::Alert::Level, which is bound as py::enum_ (inherits class_) with default holder = std::unique_ptr.
  5. Casting that shared_ptr into a Python Level object goes through a path that assumes holder compatibility; for non-smart-holder
    target types it eventually treats holder memory as if it were unique_ptr.
  6. That is UB; later deallocation runs unique_ptr delete on invalid data/pointer, producing free(): invalid size.

So pybind is not “randomly” deleting Level; it’s getting put into an invalid ownership state via holder-type mismatch.

Summarizing code locations with excerpts

Absolutely — here are the exact places.

1) def_readwrite routes through property_cpp_function<type, D>::read

  class_ &def_readwrite(const char *name, D C::*pm, const Extra &...extra) {                                                                 
      ...                                                                                                                                    
      def_property(name,                                                                                                                     
                   property_cpp_function<type, D>::read(pm, *this),                                                                          
                   property_cpp_function<type, D>::write(pm, *this),                                                                         
                   return_value_policy::reference_internal,                                                                                  
                   extra...);                                                                                   an expli                             
  }

2) For smart-holder classes with by-value members, read returns std::shared_ptr

  template <typename T, typename D>                                                                                                          
  struct property_cpp_function_sh_member_held_by_value {                                                                                     
      ...                                                                                                                                    
      static cpp_function read(PM pm, const handle &hdl) {                                                                                   
          type_info *tinfo = get_type_info(typeid(T), /*throw_if_missing=*/true);                                                            
          if (tinfo->holder_enum_v == holder_enum_t::smart_holder) {                                                                         
              return cpp_function(                                                                                                           
                  [pm](handle c_hdl) -> std::shared_ptr<D> {                                                                                 
                      std::shared_ptr<T> c_sp                                                                                                
                          = type_caster<std::shared_ptr<T>>::shared_ptr_with_responsible_parent(c_hdl);                                      
                      return std::shared_ptr<D>(c_sp, &(c_sp.get()->*pm));                                                                   
                  },                                                                                                                         
                  is_method(hdl));                                                                                                           
          }                                                                                                                                  
          return property_cpp_function_classic<T, D>::read(pm, hdl);                                                                         
      }                                                                                                                                      
  }

That is exactly the std::shared_ptr(c_sp, &(c_sp.get()->*pm)) aliasing getter.

3) This specialization is selected for by-value non-pointer/non-smart-pointer members

  struct property_cpp_function<...>                                                                                                          
      : detail::property_cpp_function_sh_member_held_by_value<T, D> {};

Suggested changelog entry:

  • Fix crash in def_readwrite for non-smart-holder properties of smart-holder classes

@virtuald virtuald changed the title Fix smart_holder def_readwrite for non-smart-holder member types Fix crash in def_readwrite for non-smart-holder properties of smart-holder classes Mar 4, 2026
@henryiii henryiii requested a review from rwgk March 6, 2026 22:05
oremanj
oremanj reviewed Mar 10, 2026
View reviewed changes
include/pybind11/pybind11.h
template <typename T, typename D>
struct property_cpp_function_sh_member_held_by_value {
static bool use_smart_holder_member_aliasing() {
type_info *tinfo = get_type_info(typeid(D), /*throw_if_missing=*/true);
Copy link
Collaborator

@oremanj oremanj Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds an implicit requirement that you must bind the type of a field before you bind a property that returns something of that type. That has never been required before, and is certainly going to trip people up.

Can we instead modify the shared_ptr to-Python cast path so that it doesn't blindly assume it's casting to a type with a compatible holder? It used to not be able to know better, but now type_info has a holder-type field. And it would save the wrong-holder UB in all cases rather than just this one.

Copy link
Contributor Author

@virtuald virtuald Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take a look this weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oremanj oremanj oremanj left review comments

@rwgk rwgk Awaiting requested review from rwgk

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@virtuald @oremanj